-
Notifications
You must be signed in to change notification settings - Fork 369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace all instances of Calendar.current with Calendar.autoupdatingCurrent #5529
Replace all instances of Calendar.current with Calendar.autoupdatingCurrent #5529
Conversation
IOS-378 Replace all instances of `Calendar.current` with `Calendar.autoupdatingCurrent`
This will not affect caching problems, or Calendar values that were cached, but if the user is frequently travelling time zones, this should make a nicer experience for them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the difference between .current
and .autoupdatingCurrent
is that the latter listens to currentLocaleDidChangeNotification
and updates the internal reference to the current calendar. Since we never hold onto the instance of a calendar, the use of .current
is appropriate and is probably lighter alternative to autoupdatingCurrent
.
Reviewable status: 0 of 6 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, we don't hold any references, so I agree that this change is probably unnecessary. @buggmagnet, as the auther of the issue, any thoughts on this?
Reviewable status: 0 of 6 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't perceive any advantages in implementing this change.
Reviewable status: 0 of 6 files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing both with .autoupdatingCurrent
and .current
I can see no changes in behaviour. Which means it also doesn't fix the original problem. Closing this PR.
Reviewable status: 0 of 6 files reviewed, all discussions resolved
Calendar.current
does not observe any user made changes to the calendar.This will not affect caching problems, or calendar values that were cached, but if the user is frequently travelling time zones, this should make a nicer experience for them.
This change is